soroban-rpc: simulateTransaction: automatically detect ledger entries which require restoring #865

2opremio · 2023-08-16T23:53:27Z

This change incorporates two new fields to simulateTransaction's Response:

PreRestoreTransactionData string                       `json:"preRestoreTransactionData,omitempty"` // SorobanTransactionData XDR in base64
PreRestoreMinResourceFee  int64                        `json:"preRestoreMinResourceFee,omitempty"`

When preflighting an InvokeHostFunction operation, soroban-rpc will now detect ledger entries which require restoring prior to the invocation for it to succeed.

If any such entries are detected, the simulateTransaction response will provide the Soroban Transaction Data and minimum resource fee needed to restore those entries.

The user is then now expected to:

Submit a RestoreFootprint operation using the new fields above, if present.
Submit the InvokeHostFunction operation normally (i.e. using the pre-existing simulateTransaction response fields)

Closes #803

… which require restoring This change incorporates two new fields to the simulateTransactionResponse: ``` PreRestoreTransactionData string `json:"preRestoreTransactionData,omitempty"` // SorobanTransactionData XDR in base64 PreRestoreMinResourceFee int64 `json:"preRestoreMinResourceFee,omitempty"` ``` When preflighting an InvokeHostFunction operation, soroban-rpc will now detect ledger entries which require restoring prior to the invocation for it to succeed. If any such entries are detected, the simulateTransaction response will provide the Soroban Transaction Data and minimum resource fee needed to restore those entries. The user is then now expected to: 1. Submit a RestoreFootprint operation using the new fields above, if present. 2. Submit the InvokeHostFunction operation normally.

2opremio · 2023-08-17T00:08:55Z

@dmkozh does something like state_expiration.rs already exist? I figure you need something like that upstream.

Otherwise, it may make sense to place it in the env library if you find it appropriate.

2opremio · 2023-08-17T00:12:18Z

cmd/soroban-rpc/internal/test/simulate_transaction_test.go

+	// Wait for expiration again and check the pre-restore field when trying to exec the contract again
+	waitForExpiration()
+
+	simulationResult := simulateTransactionFromTxParams(t, client, invokeIncPresistentEntryParams)
+	assert.NotZero(t, simulationResult.PreRestoreTransactionData)
+	assert.NotZero(t, simulationResult.PreRestoreMinResourceFee)
+
+	params = preflightTransactionParamsLocally(t,
+		txnbuild.TransactionParams{
+			SourceAccount:        &account,
+			IncrementSequenceNum: true,
+			Operations: []txnbuild.Operation{
+				&txnbuild.RestoreFootprint{},
+			},
+			Preconditions: txnbuild.Preconditions{
+				TimeBounds: txnbuild.NewInfiniteTimeout(),
+			},
+		},
+		methods.SimulateTransactionResponse{
+			TransactionData: simulationResult.PreRestoreTransactionData,
+			MinResourceFee:  simulationResult.PreRestoreMinResourceFee,
+		},
+	)
+	tx, err = txnbuild.NewTransaction(params)
+	assert.NoError(t, err)
+	sendSuccessfulTransaction(t, client, sourceAccount, tx)
+
+	// Finally, we should be able to send the inc host function invocation now that we
+	// have pre-restored the entries
+	params = preflightTransactionParamsLocally(t, invokeIncPresistentEntryParams, simulationResult)
+	tx, err = txnbuild.NewTransaction(params)
+	assert.NoError(t, err)
+	sendSuccessfulTransaction(t, client, sourceAccount, tx)
 }


@Shaptic here's an example of how to use it.

Thank you! This means the flow is something like:

result = simulateTransaction(invoke) if result.PreRestoreTransactionData != undefined: submitTransaction(restore, transactionData=result.PreRestoreTransactionData) result = simulateTransaction(invoke) submitTransaction(invoke, transactionData=result.transactionData)

Correct?

Yes, assuming you somehow also use the minfee too.

Actually, you don't need the second simulation

result = simulateTransaction(invoke) if result.PreRestoreTransactionData != undefined: submitTransaction(restore, transactionData=result.PreRestoreTransactionData) submitTransaction(invoke, transactionData=result.transactionData)

@Shaptic did you see my udpate above? You don't need to re-run the transaction simulation

tsachiherman · 2023-08-17T00:36:14Z

To me, PreRestore sounds misleading, since it's not something you would do before the restore operation. it's the information for the restore operation. I would use the preamble/prelude prefix for these two fields.
@Shaptic any thoughts about the naming and/or implementation ?

2opremio · 2023-08-17T00:46:50Z

@willemneal we should probably use this in the cli once merged

2opremio · 2023-08-17T13:13:32Z

@Shaptic you have an example of how to use this at

To me, PreRestore sounds misleading, since it's not something you would do before the restore operation. it's the information for the restore operation. I would use the preamble/prelude prefix for these two fields.

No strong opinions here.

tsachiherman · 2023-08-17T16:44:03Z

thinking about that further, I think that what I'd like to see if I were in @Shaptic 's seat is the following:

RestorePreamble  restorePreamble   `json:"restorePeramble,omitempty"` // optional restore preamble

where the restorePreamble is defined as

type restorePreamble struct {
  TransactionData string           `json:"transactionData"` // non-optional since the struct itself is optional; base64 encoded.
  MinResourceFee  int64            `json:"minResourceFee"` // non-optional since the struct itself is optional.
}

Shaptic · 2023-08-17T16:55:58Z

@tsachiherman from a client perspective, having one omittable structure is better than two, so yes, I agree - I like encapsulating it into a structure like this. Of course we could have said "zero or both of these are present," but "zero or one" is much better!

Shaptic

you rock 🎸

this LGTM as-is but I’d prefer the schema Tsachi outlined

2opremio · 2023-08-18T18:24:01Z

this LGTM as-is but I’d prefer the schema Tsachi outlined

sure

cmd/soroban-rpc/internal/methods/simulate_transaction.go

Co-authored-by: George <Shaptic@users.noreply.github.com>

2opremio requested review from Shaptic, dmkozh, paulbellamy and sydneynotthecity August 16, 2023 23:53

2opremio force-pushed the detect-required-restores branch from 6cf6d6f to fafe104 Compare August 16, 2023 23:56

2opremio force-pushed the detect-required-restores branch from fafe104 to ebe29e4 Compare August 16, 2023 23:56

2opremio commented Aug 17, 2023

View reviewed changes

tsachiherman assigned 2opremio Aug 17, 2023

tsachiherman added the soroban-scrum label Aug 17, 2023

tsachiherman added this to the Soroban Testnet Release milestone Aug 17, 2023

Shaptic approved these changes Aug 17, 2023

View reviewed changes

2opremio added 2 commits August 18, 2023 20:39

Address review feedback

c7e41dd

Merge branch 'main' into detect-required-restores

1907575

tsachiherman approved these changes Aug 18, 2023

View reviewed changes

tsachiherman reviewed Aug 18, 2023

View reviewed changes

cmd/soroban-rpc/internal/methods/simulate_transaction.go Show resolved Hide resolved

Shaptic reviewed Aug 18, 2023

View reviewed changes

cmd/soroban-rpc/internal/methods/simulate_transaction.go Outdated Show resolved Hide resolved

2opremio and others added 2 commits August 19, 2023 01:05

Merge branch 'main' into detect-required-restores

0f6bb5d

Update cmd/soroban-rpc/internal/methods/simulate_transaction.go

f8952a3

Co-authored-by: George <Shaptic@users.noreply.github.com>

2opremio enabled auto-merge (squash) August 18, 2023 23:21

2opremio merged commit 08de7d5 into stellar:main Aug 18, 2023
19 of 20 checks passed

2opremio deleted the detect-required-restores branch August 18, 2023 23:47

2opremio mentioned this pull request Aug 19, 2023

Expiration Entry rework stellar/stellar-xdr#137

Merged

Shaptic mentioned this pull request Aug 23, 2023

Handle new RPC simulation response variations stellar/js-soroban-client#132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soroban-rpc: simulateTransaction: automatically detect ledger entries which require restoring #865

soroban-rpc: simulateTransaction: automatically detect ledger entries which require restoring #865

2opremio commented Aug 16, 2023 •

edited

Loading

2opremio commented Aug 17, 2023

2opremio Aug 17, 2023

Shaptic Aug 17, 2023

2opremio Aug 18, 2023

2opremio Aug 18, 2023

2opremio Aug 22, 2023

tsachiherman commented Aug 17, 2023 •

edited

Loading

2opremio commented Aug 17, 2023

2opremio commented Aug 17, 2023

tsachiherman commented Aug 17, 2023

Shaptic commented Aug 17, 2023

Shaptic left a comment •

edited

Loading

2opremio commented Aug 18, 2023

soroban-rpc: simulateTransaction: automatically detect ledger entries which require restoring #865

soroban-rpc: simulateTransaction: automatically detect ledger entries which require restoring #865

Conversation

2opremio commented Aug 16, 2023 • edited Loading

2opremio commented Aug 17, 2023

2opremio Aug 17, 2023

Choose a reason for hiding this comment

Shaptic Aug 17, 2023

Choose a reason for hiding this comment

2opremio Aug 18, 2023

Choose a reason for hiding this comment

2opremio Aug 18, 2023

Choose a reason for hiding this comment

2opremio Aug 22, 2023

Choose a reason for hiding this comment

tsachiherman commented Aug 17, 2023 • edited Loading

2opremio commented Aug 17, 2023

2opremio commented Aug 17, 2023

tsachiherman commented Aug 17, 2023

Shaptic commented Aug 17, 2023

Shaptic left a comment • edited Loading

Choose a reason for hiding this comment

2opremio commented Aug 18, 2023

2opremio commented Aug 16, 2023 •

edited

Loading

tsachiherman commented Aug 17, 2023 •

edited

Loading

Shaptic left a comment •

edited

Loading